fix: normalize same-origin absolute URLs for client-side navigation#336
Conversation
When <Link href="https://example.com/path"> is used and example.com matches the current origin, vinext now does SPA navigation instead of a full page reload. This matches Next.js behavior. Adds toSameOriginPath() utility that extracts the local path from a same-origin absolute URL. Applied consistently across: - link.tsx handleClick() and prefetch paths - navigation.ts navigateImpl() (App Router) - router.ts push()/replace() (Pages Router hook + singleton) Closes cloudflare#335
commit: |
There was a problem hiding this comment.
Pull request overview
This PR fixes client-side navigation for same-origin absolute URLs (e.g. http://localhost:3000/about) by normalizing them to local paths before “external URL” bail-outs, aligning behavior with Next.js and resolving #335.
Changes:
- Added
toSameOriginPath()utility to convert same-origin absolute URLs topathname + search + hash(or returnnullfor external/server contexts). - Updated Link, App Router navigation, and Pages Router
push/replaceto normalize same-origin absolute URLs before delegating to full-page navigation. - Added unit tests for
toSameOriginPath()and SSR rendering checks for<Link>with absolute URLs.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tests/link.test.ts |
Adds tests for same-origin normalization utility and SSR output for absolute hrefs. |
packages/vinext/src/shims/url-utils.ts |
Introduces shared toSameOriginPath() helper used by multiple navigation shims. |
packages/vinext/src/shims/link.tsx |
Normalizes same-origin absolute URLs for prefetch/click handling to enable SPA navigation. |
packages/vinext/src/shims/navigation.ts |
Normalizes same-origin absolute URLs before external URL handling in App Router navigation. |
packages/vinext/src/shims/router.ts |
Normalizes same-origin absolute URLs before external URL handling in Pages Router push/replace. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Use window.location.origin as base for `//` URLs so they resolve correctly for same-origin detection, while keeping `new URL(url)` (no base) for http/https URLs to preserve existing behavior. Addresses Copilot review comment on PR cloudflare#336.
|
/bigbonk review |
There was a problem hiding this comment.
Clean, well-scoped fix. The toSameOriginPath() utility is correct and the integration across all navigation paths (Link, App Router, Pages Router hook + singleton) is thorough and consistent.
All tests pass (36 link tests, 568 shim tests), typecheck and lint are clean.
Two minor observations below — neither blocks merge.
| if (!observer) return; | ||
|
|
||
| observerCallbacks.set(node, () => prefetchUrl(localizedHref)); | ||
| observerCallbacks.set(node, () => prefetchUrl(hrefToPrefetch)); |
There was a problem hiding this comment.
Minor: prefetchUrl() already normalizes same-origin URLs internally (lines 150-156). When the IntersectionObserver fires this callback, toSameOriginPath will be called a second time inside prefetchUrl. This is harmless (cheap, idempotent, correct) but the double normalization could be avoided by passing hrefToPrefetch here, which you already do — so it only re-normalizes if the href was already local. Just noting the interaction; no change needed.
| // External links: let the browser handle it. | ||
| // Same-origin absolute URLs (e.g. http://localhost:3000/about) are | ||
| // normalized to local paths so they get client-side navigation. | ||
| let navigateHref = resolvedHref; |
There was a problem hiding this comment.
Nice detail: using resolvedHref (which incorporates the as prop) for the external check, and then feeding the normalized path into resolveRelativeHref / withBasePath downstream. The as-prop case is handled correctly because as would typically be a local path, but if someone passes an absolute URL as as, this now does the right thing.
|
Review submitted. Here's a summary of my findings: Approved. This is a clean, well-scoped fix for #335. What the PR does: Adds a What I verified:
One minor suggestion: The test comment on line 247 of |
Co-authored-by: ask-bonk[bot] <249159057+ask-bonk[bot]@users.noreply.github.com>
|
Nice one! |
Summary
Fixes #335
toSameOriginPath()utility that extracts the local path (pathname + search + hash) from a same-origin absolute URL, returningnullfor truly external URLs or server-side contextslink.tsxhandleClick()and prefetch paths to normalize same-origin URLs before the external URL bail-outnavigation.tsnavigateImpl()(App Router) to normalize before external URL checkrouter.tspush()/replace()in both theuseRouterhook andRoutersingleton (Pages Router)Root Cause
isExternalUrl()treated any URL with a scheme as external — no same-origin comparison. When<Link href="http://localhost:3000/about">was used and the origin matched, it triggered a full page reload instead of SPA navigation.Test plan
toSameOriginPath()— same-origin returns local path, cross-origin returns null, server returns null, preserves search/hash<Link>with absolute URLsisExternalUrl()still correctly identifies truly external URLspnpm test tests/link.test.ts— 35 tests passpnpm test tests/shims.test.ts— 568 tests passpnpm run typecheck— cleanpnpm run lint— clean